[management] Add idp timeout env variable #4647
Conversation
ressys1978
changed the title
… recorder is not available (netbirdio#4652)
|
@mlsmaycon how does one get this reviewed? |
|
@bcmmbaga can you have a look? |
WalkthroughIntroduces environment-variable-driven timeout configuration for IDP HTTP clients across all manager implementations. Replaces hardcoded 10-second timeouts with a dynamic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
management/server/idp/pocketid.go (1)
89-93: Consider consistent formatting across all IDP implementations.Some IDP files (okta.go, jumpcloud.go) don't have the extra blank line after
httpClientinitialization, while others do (azure.go, zitadel.go, google_workspace.go, keycloak.go, auth0.go, pocketid.go). Consider applying uniform formatting for consistency.management/server/idp/util.go (1)
82-94: Core implementation is correct, but consider adding logging for parse errors.The function correctly reads the environment variable and falls back to the default timeout when unset or unparseable. However, when
time.ParseDurationfails, the function silently returns the default without logging. This could make debugging difficult if someone sets an invalid value.Consider adding a log warning when parse fails:
func idpTimeout() time.Duration { timeoutStr, ok := os.LookupEnv(idpTimeoutEnv) if !ok || timeoutStr == "" { return defaultTimeout } timeout, err := time.ParseDuration(timeoutStr) if err != nil { + log.Warnf("invalid %s value %q, using default %s: %v", idpTimeoutEnv, timeoutStr, defaultTimeout, err) return defaultTimeout } return timeout }Note: You'll need to import the log package at the top of the file:
log "github.com/sirupsen/logrus"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
management/server/idp/auth0.go(1 hunks)management/server/idp/authentik.go(1 hunks)management/server/idp/azure.go(1 hunks)management/server/idp/google_workspace.go(1 hunks)management/server/idp/jumpcloud.go(1 hunks)management/server/idp/keycloak.go(1 hunks)management/server/idp/okta.go(1 hunks)management/server/idp/pocketid.go(1 hunks)management/server/idp/util.go(2 hunks)management/server/idp/zitadel.go(1 hunks)
🔇 Additional comments (11)
management/server/idp/jumpcloud.go (1)
47-51: LGTM!The timeout configuration change is consistent with the broader IDP timeout refactoring.
management/server/idp/zitadel.go (1)
166-170: LGTM!The dynamic timeout configuration is correctly applied while preserving the existing transport configuration.
management/server/idp/azure.go (1)
60-64: LGTM!The timeout configuration aligns with the centralized IDP timeout management approach.
management/server/idp/google_workspace.go (1)
50-54: LGTM!The dynamic timeout implementation is correctly integrated into the Google Workspace manager initialization.
management/server/idp/keycloak.go (1)
65-69: LGTM! Directly addresses the reported Keycloak timeout issue.This change enables users to configure longer timeouts for large Keycloak environments with LDAP federation, resolving the "context deadline exceeded" errors reported in issue #1386.
management/server/idp/auth0.go (1)
138-142: LGTM!The timeout configuration change is properly integrated into the Auth0 manager initialization.
management/server/idp/pocketid.go (1)
89-93: LGTM!The timeout configuration is consistently applied.
management/server/idp/okta.go (1)
46-49: ✓ Verification complete—idpTimeout()implementation is correct and properly handles errors.The function exists in
management/server/idp/util.go(lines 83–94) and:
- Reads the environment variable
NB_IDP_TIMEOUT- Returns a default timeout of 10 seconds if the env var is missing, empty, or contains an invalid duration format
- Includes proper error handling with fallback behavior
The
idpTimeout()call in the okta.go changes is sound.management/server/idp/authentik.go (1)
51-59: LGTM! Timeout now configurable via environment variable.The change from hardcoded
10 * time.SecondtoidpTimeout()successfully makes the HTTP client timeout configurable through theNB_IDP_TIMEOUTenvironment variable, addressing the issue with large Keycloak setups that may require longer timeouts.management/server/idp/util.go (2)
7-9: LGTM! Necessary imports for environment-driven timeout.The
osandtimeimports are correctly added to support the newidpTimeout()functionality.
75-80: LGTM! Constants follow review feedback.The constants correctly implement the suggestions from previous reviews: using the
NB_prefix for consistency and definingdefaultTimeoutas a constant to avoid re-parsing.
|
how soon will this fix appear in the main branch? |
8 participants
Describe your changes
Introduced the NETBIRD_IDP_TIMEOUT environment variable to the management service. This allows configuring a timeout for supported IDPs. If the variable is unset or contains an invalid value, a default timeout of 10 seconds is used as a fallback.
This is needed for larger IDP environments where 10s is just not enough time.
Issue ticket number and link
This should fix #1386
Stack
Checklist
Documentation
Select exactly one:
I don't feel this needs to be documented at this time, as currently the number of self-hosted netbird users with larger IDP environments is probably very small.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit